fix: replace deprecated golang.org/x/net/websocket with gorilla/websocket#45
fix: replace deprecated golang.org/x/net/websocket with gorilla/websocket#45
Conversation
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…cket Agent-Logs-Url: https://github.com/Automattic/cron-control-runner/sessions/c1de8e80-9bd6-4c70-8b70-8f15cae35c63 Co-authored-by: sjinks <7810770+sjinks@users.noreply.github.com>
sjinks
force-pushed
the
copilot/sub-pr-43
branch
from
f949791 to
ed2073c
Compare
|
@copilot, are the changes covered by tests? |
Agent-Logs-Url: https://github.com/Automattic/cron-control-runner/sessions/92142be9-6f84-4c2f-8bbe-4b29a1ca3652 Co-authored-by: sjinks <7810770+sjinks@users.noreply.github.com>
Added in commit e06889a (
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the deprecated golang.org/x/net/websocket usage in the remote package with github.com/gorilla/websocket, while preserving the existing net.Conn-based byte-stream interface via a small adapter.
Changes:
- Introduces
wsNetConnto adapt*gorilla.Conntonet.Connsemantics (buffering full WS messages and chunking onRead, serializingWrite). - Updates the WebSocket listener to use
gorilla.Upgrader+http.HandlerFunc, and updates shutdown handling to send a close control frame via the adapter. - Adds a new
wsnetconn_test.gotest suite and updates module dependencies for gorilla/websocket.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| remote/remote.go | Replaces x/net/websocket server handling with gorilla/websocket, adds wsNetConn, updates shutdown close logic |
| remote/wsnetconn_test.go | Adds tests for wsNetConn read/write behavior, deadlines, close frames, and concurrency |
| go.mod | Adds gorilla/websocket as a direct dependency; demotes x/net to indirect |
| go.sum | Adds checksums for gorilla/websocket |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ws, err := upgrader.Upgrade(w, r, nil) | ||
| if err != nil { | ||
| t.Errorf("upgrade: %v", err) | ||
| return | ||
| } | ||
| serverConnCh <- ws | ||
| <-handlerDone // keep the handler alive until cleanup | ||
| })) | ||
|
|
||
| wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + "/" | ||
| clientWS, _, err := gorilla.DefaultDialer.Dial(wsURL, nil) | ||
| if err != nil { | ||
| close(handlerDone) | ||
| srv.Close() | ||
| t.Fatalf("dial: %v", err) | ||
| } | ||
|
|
||
| serverWS := <-serverConnCh | ||
|
|
There was a problem hiding this comment.
newTestWSPair can deadlock indefinitely if the server-side upgrader.Upgrade fails (handler returns after t.Errorf without sending to serverConnCh, while the client goroutine blocks on <-serverConnCh). Consider propagating the upgrade error via a channel and/or using a select with timeout when waiting for serverWS so the test fails fast instead of hanging.
| client.SetReadDeadline(time.Now().Add(50 * time.Millisecond)) | ||
| client.ReadMessage() //nolint:errcheck // best-effort drain | ||
| } |
There was a problem hiding this comment.
The drain goroutine busy-loops due to select { default: ... } and will repeatedly set deadlines and call ReadMessage as fast as possible, which can spike CPU and introduce test flakiness. Consider using a ticker/sleep, or block on ReadMessage with a deadline and handle timeout errors, while still honoring the stop channel.
| client.SetReadDeadline(time.Now().Add(50 * time.Millisecond)) | |
| client.ReadMessage() //nolint:errcheck // best-effort drain | |
| } | |
| } | |
| client.SetReadDeadline(time.Now().Add(50 * time.Millisecond)) | |
| if _, _, err := client.ReadMessage(); err != nil { | |
| if netErr, ok := err.(net.Error); ok && netErr.Timeout() { | |
| continue | |
| } | |
| return | |
| } |
| func (c *wsNetConn) Read(b []byte) (int, error) { | ||
| for len(c.readBuf) == 0 { | ||
| _, msg, err := c.conn.ReadMessage() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| c.readBuf = msg | ||
| } | ||
| n := copy(b, c.readBuf) | ||
| c.readBuf = c.readBuf[n:] | ||
| return n, nil |
There was a problem hiding this comment.
wsNetConn.Read currently returns gorilla's close/EOF errors directly. Downstream code in this package checks for io.EOF specifically when the client disconnects; a *websocket.CloseError will be treated as an unexpected error and logged noisily. Consider translating normal close conditions (CloseError / unexpected EOF) to io.EOF (or net.ErrClosed) to preserve the previous net.Conn-like semantics.
| // wsNetConn wraps a *gorilla.Conn and implements the net.Conn interface so that | ||
| // the WebSocket connection can be used wherever a plain net.Conn is expected. | ||
| // gorilla/websocket is message-oriented, so Read buffers an entire message and | ||
| // returns chunks of it on successive calls. | ||
| type wsNetConn struct { | ||
| conn *gorilla.Conn | ||
| mu sync.Mutex // serialises concurrent writes (gorilla requires one writer at a time) | ||
| readBuf []byte | ||
| } | ||
|
|
||
| func newWSNetConn(conn *gorilla.Conn) *wsNetConn { | ||
| return &wsNetConn{conn: conn} | ||
| } | ||
|
|
||
| func (c *wsNetConn) Read(b []byte) (int, error) { | ||
| for len(c.readBuf) == 0 { | ||
| _, msg, err := c.conn.ReadMessage() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| c.readBuf = msg | ||
| } | ||
| n := copy(b, c.readBuf) | ||
| c.readBuf = c.readBuf[n:] | ||
| return n, nil | ||
| } |
There was a problem hiding this comment.
wsNetConn is presented as a net.Conn implementation, but Read is not concurrency-protected. gorilla/websocket explicitly allows only one concurrent reader; if two goroutines call Read concurrently (which net.Conn permits), this can race/panic. Consider adding a read-side mutex (or otherwise enforcing single-reader usage) to make the adapter safe for concurrent net.Conn use.
| func (c *wsNetConn) Read(b []byte) (int, error) { | ||
| for len(c.readBuf) == 0 { | ||
| _, msg, err := c.conn.ReadMessage() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| c.readBuf = msg | ||
| } |
There was a problem hiding this comment.
wsNetConn.Read uses ReadMessage, which buffers an entire WebSocket message into memory before returning any bytes. Without a read limit, a peer can send a very large frame/message and force large allocations (potential DoS). Consider setting a maximum message size (e.g., via conn.SetReadLimit) when creating/accepting the WebSocket connection.
| wsConn, ok := conn.(*wsNetConn) | ||
| if ok { | ||
| wsConn.WriteClose(shutdownErrorCode) | ||
| wsConn.writeClose(shutdownErrorCode) |
There was a problem hiding this comment.
processShutdown ignores the error returned by writeClose. If the close control frame fails to send (e.g., broken connection), it may be useful to log the error for diagnosis during shutdown.
| wsConn.writeClose(shutdownErrorCode) | |
| if err := wsConn.writeClose(shutdownErrorCode); err != nil { | |
| log.Printf("remote: failed to send websocket close frame during shutdown. GUID: %s, err: %v\n", wpcli.GUID, err) | |
| } |
golang.org/x/net/websocketis effectively deprecated by the Go team. This replaces it withgithub.com/gorilla/websocket v1.5.3, a stable and actively maintained alternative.Changes
wsNetConnadapter (remote/remote.go): wraps*gorilla.Connas anet.Connto preserve the existing byte-stream interface used throughout the package. Gorilla is message-oriented, soRead()buffers full messages and returns chunks;Write()is mutex-protected (gorilla enforces one concurrent writer).ListenForConnections(): replacedwebsocket.Handler(...)withgorilla.Upgrader+http.HandlerFunc.CheckOriginset to allow all origins (internal-only service).processShutdown(): type assertion updated from*websocket.Conn→*wsNetConn; close frame sent viawriteClose()usinggorilla.WriteControl.go.mod:gorilla/websocket v1.5.3added as direct dep;golang.org/x/netdemoted to indirect (still pinned at the security-patchedv0.38.0).remote/wsnetconn_test.go): new test file coveringRead(single message, chunked, multiple messages),Write, concurrent write safety,writeCloseclose-code verification,LocalAddr/RemoteAddr,SetReadDeadlinetimeout behaviour, and a compile-timenet.Conninterface check.